-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Check for failures in DT CI #10544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Check for failures in DT CI #10544
Conversation
|
/cmd prdoc --audience runtime_dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to us jq instead of a python script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that the Python script is a bit longer than JQ, but I think that it's more readable than it when it comes to what we're trying to do here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that end up in a pretty unreadable CLI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't try it but I think it should be a simple oneliner.
Better have the test-suite return status e.g. number of tests failed -> 0 means success. IF that is easy to make
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really care
but this do seems to be a bit of an overkill if we can just do
report_file="$1"
if jq -e '
any(
.execution_information[]
| .case_reports[]
| .mode_execution_reports[]
| .status.status;
. == "Failed"
)
' $report_file; then
exit 1
else
exit 0
fi
athei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to have a 87 line parsing script to determine if the test suite succeeded? Can't we just return proper status codes when calling the test suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that end up in a pretty unreadable CLI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really care
but this do seems to be a bit of an overkill if we can just do
report_file="$1"
if jq -e '
any(
.execution_information[]
| .case_reports[]
| .mode_execution_reports[]
| .status.status;
. == "Failed"
)
' $report_file; then
exit 1
else
exit 0
fi
In the future we could. But for the time being, we expect some of the tests in the PolkaVM test suite to fail and we expect all of the tests in the REVM test suite to succeed. So, I think that making retester return a status code based on failures is very doable but I think it makes more sense once all of the tests for REVM and PolkaVM pass and we no longer need to have this conditional logic. Also, a very big portion of this script is just models that I copied over from another script that I wrote. We technically don't need this whole code, all that we need is the following: with open(sys.argv[1], "r") as file:
report: Report = json.load(file)
for _, mode_to_case_mapping in report["execution_information"].items():
for _, case_idx_to_report_mapping in mode_to_case_mapping[
"case_reports"
].items():
for _, execution_report in case_idx_to_report_mapping[
"mode_execution_reports"
].items():
if execution_report["status"]["status"] == "Failed":
exit(1)
exit(0) |
Differential Tests Results (PolkaVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
Differential Tests Results (REVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
I don't get how this is related. The CI script would allow non zero status code for PVM but not for EVM. That is exactly what we are doing here just with extra steps (the parsing). We could also return the number of failing tests as status code. Determining whether the tests were OK or FAIL should easy. |
Description
This is a small PR that makes the CI of the differential testing framework fail if any failure is encountered. We only do this with the
revive-dev-node-revm-solctarget since this is the only target at the moment that has no failures.